No special font-locking for mixedCase, CamelCase #462

Bost · 2017-11-06T18:04:16Z

This is an alternative approach to my PR regarding fix-namespace-font-locking.
I find out the clojure-interop-method-face is not really needed for mixedCase, CamelCase... and it simplifies the regexp a lot. What do you think about that?
In case you think the mixedCase, CamelCase are needed indeed, then I'd build-up/rework their pattern matching based on this simplification.

bbatsov · 2017-11-13T18:13:58Z

Perhaps this is the better approach indeed. We'll lose the special font-locking for interop methods, but it didn't seem particularly important to begin with.

bbatsov · 2017-11-13T18:15:37Z

test/clojure-mode-font-lock-test.el

@@ -161,7 +161,7 @@ POS."
    (should (eq (clojure-test-face-at 2 3) 'font-lock-type-face))
    (should (eq (clojure-test-face-at 5 14) 'font-lock-type-face))))

-(ert-deftest clojure-mode-syntax-table/namespace ()
+(ert-deftest clojure-mode-syntax-table/segmented-symbol ()


It seems to me that those tests should actually fail now - from what I gather by your changes we aim to font-lock as ns-es only ns symbols that are in the ns macro.

Perhaps this is the better approach indeed.

Glad to hear it :)

It seems to me that those tests should actually fail now - from what I gather by your changes we aim to font-lock as ns-es only ns symbols that are in the ns macro.

Yea, I'll work on it. (I just wanted to hear your general opinion in the first place.) I'll tell you when have something to show.

Bost · 2017-11-19T23:39:25Z

I made a good progress. Please have a look. Thx.

bbatsov · 2017-12-03T08:34:39Z

clojure-mode.el

-      ;; foo.bar.baz
-      ("\\<^?\\([a-z][a-z0-9_-]+\\.\\([a-z][a-z0-9_-]*\\.?\\)+\\)" 1 font-lock-type-face)
-      ;; (ns namespace) - special handling for single segment namespaces
+
      (,(concat "(\\<ns\\>[ \r\n\t]*"


Add some comment for each regular expression, otherwise it's hard to understand their purpose.

Comment added.

bbatsov · 2017-12-03T08:35:40Z

clojure-mode.el

+                "\\(" clojure--sym-regexp "\\)")
+       (1 font-lock-type-face))
+
+      (,(concat "\\(:\\)\\(" clojure--sym-regexp "\\)\\(/\\)\\(" clojure--sym-regexp "\\)")


I'm quite puzzled by this regexp.

Comment added.

bbatsov · 2017-12-03T08:37:35Z

clojure-mode.el

+       (2 'clojure-keyword-face))
+
+      ;; type-hints
+      (,(concat "\\(#^\\)\\(" clojure--sym-regexp "\\)\\(/\\)\\(" clojure--sym-regexp "\\)")


I guess the first is some qualified symbol, but certainly some examples would make the intent clearer.

See the test.clj

I meant there should be some examples here as well, so it's easier to map those regular expressions to what they are supposed to font-lock.

I meant there should be some examples here

(simple) examples added. More elaborate examples are in the test.clj

bbatsov · 2017-12-03T08:42:09Z

I see 3 tests are failing on Travis. This also needs a changelog entry.

bbatsov · 2017-12-03T08:43:04Z

test.clj

+   [veryCom|pLex.stu-ff]))
+
+;; TODO the face of 'fn' is 't'. Where is it defined?
+(fn foo [x] x)


fn is in the list of special forms, so it should be font-locked as one. I can't imagine what can cause the problem you've mentioned. At least I don't see anything obvious in your changes.

I need to have a look at it. Give me some time please.

bbatsov · 2017-12-03T08:48:50Z

clojure-mode.el

-      ("\\<^?\\(:\\(\\sw\\|\\s_\\)+\\(\\>\\|\\_>\\)\\)" 1 'clojure-keyword-face append)
-      ;; Java interop highlighting
-      ;; CONST SOME_CONST (optionally prefixed by /)
-      ("\\(?:\\<\\|/\\)\\([A-Z]+\\|\\([A-Z]+_[A-Z1-9_]+\\)\\)\\>" 1 font-lock-constant-face)


And I guess no special font-locking for SCREAMING_UPPER_CASE.

Right, no special font-locking for SCREAMING_UPPER_CASE at the moment. But feel free to change or propose something else.

I'm just mentioning it. I don't feel strongly about this.

bbatsov · 2018-01-01T06:30:24Z

@Bost How is all of this shaping up?

Bost · 2018-01-01T15:31:23Z

I see 3 tests are failing on Travis. This also needs a changelog entry.

I'm not sure how to document it. I (partially) fixed one of them, so:

clojure-find-ns-test fails only in emacs-24.4-travis, emacs-24.5-travis
clojure-mode-syntax-table/characters, clojure-mode-syntax-table/comment-macros fail in every 4 travis builds, however they pass correctly on my machine (tested in 25.2 and also 25.3). Could it be that we have a glitch in the travis somewhere? Could you try to test it on your machine please?

@Bost How is all of this shaping up?

In general, I've been using this branch for 2+ weeks w/o any problem. I think the RC1 is done.

Bost · 2018-01-23T01:12:08Z

... I just stumbled upon an interesting article about https://en.wikipedia.org/wiki/Hibernation, hmm.

bbatsov · 2018-01-23T07:34:58Z

CHANGELOG.md

@@ -21,6 +21,7 @@
 * [#429](https://github.com/clojure-emacs/clojure-mode/issues/429): Fix a bug causing last occurrence of expression sometimes is not replaced when using `move-to-let`.
 * [#423](https://github.com/clojure-emacs/clojure-mode/issues/423): Make `clojure-match-next-def` more robust against zero-arity def-like forms.
 * [#451](https://github.com/clojure-emacs/clojure-mode/issues/451): Make project root directory calculation customized by `clojure-project-root-function`.
+* Numerous font-locking fixes and improvements


List here the biggest user-visible changes explicitly and I guess we can merge this.

bbatsov · 2018-01-23T07:44:09Z

clojure-find-ns-test fails only in emacs-24.4-travis, emacs-24.5-travis

Does this work for you? I'm assuming the failure must be related to changing the ns regexp or something.

I (partially) fixed one of them, so:

Define partially. :-)

As for documenting it - I'd stick to high-level. Something like:

stop font-locking interop methods, because of this and that
stop font-locking constants because of this and that
change ns font-locking in this than manner, etc

You can also post here some screenshot of before and after for people who'll stumble upon this ticket.

And sorry about the slow responses, but I've been crazy busy lately and I spend my little spare time focusing on some long overdue CIDER improvements.

Bost · 2018-01-27T22:07:27Z

Examples of valid namespace definitions: before / after (i.e. with and without this change):

bbatsov · 2018-01-28T14:23:14Z

Yeah, the ns part certainly looks good. I was also referring to the other things affected by this - e.g. constants and interop methods.

Anyways, update the changelog to be more granular and I guess we can merge this and gather more feedback for the users.

bbatsov · 2018-03-12T01:25:37Z

@Bost ping :-)

* s/clojure-interop-method-face/font-lock-type-face * keyword font-locking * visual examples w/ ert tests * fix failing ert tests

Bost · 2018-03-13T00:23:57Z

As for documenting it - I'd stick to high-level. Something like:

I've done it. See the CHANGELOG.md

I (partially) fixed one of them, so:

Define partially. :-)

Oh BTW I was able to fix the failing emacs-25.1-travis and emacs-25.2-travis builds. The failing clojure-mode-syntax-table/characters - that was my mistake but the other one clojure-mode-syntax-table/comment-macros:

(should (equal (clojure-test-face-at
                  1 2  "#_ \n;; some crap\n (lala 0101\n lao\n\n 0 0i)")
                  'default))

that was IMO faulty. The '#_' char sequence wasn't matched by any font-locking regex so then its face should be default instead of nil.

Another thing: I wonder how was my predecessor able to pass the travis tests without ant long-line warning? See https://travis-ci.org/clojure-emacs/clojure-mode/jobs/352241172 @alexander-yakushev ?

bbatsov · 2018-03-13T13:20:48Z

Another thing: I wonder how was my predecessor able to pass the travis tests without ant long-line warning? See https://travis-ci.org/clojure-emacs/clojure-mode/jobs/352241172 @alexander-yakushev ?

Just another odd Travis problem I guess.

Thanks for tackling this!

vspinu · 2018-10-24T18:11:37Z

This shaved the highlighting of type hints. I find it much harder to read inter-op code now because types are easily confused with arguments. Any objection of putting font-lock-type-face back on type hints?

vspinu · 2018-10-24T18:14:35Z

Actually the PR has provisioned type face for hints, but they don't work for me starting with this PR.

bbatsov · 2018-10-24T18:29:12Z

@vspinu I can confirm this. I guess you can file another ticket so we won't forget to fix this.

clojure-emacs#462

#462

Bost · 2018-11-06T13:53:33Z

@vspinu I can confirm this. I guess you can file another ticket so we won't forget to fix this.

Could you show some screenshots please? Or even better add a new test case. I haven't done much SW development in the last X weeks. I need to update myself at first. Thanks.

vspinu · 2018-11-06T15:29:52Z

The type hints were fixed in dd8a193.

bbatsov reviewed Nov 13, 2017

View reviewed changes

bbatsov mentioned this pull request Nov 14, 2017

Fix namespace font locking #461

Closed

5 tasks

bbatsov reviewed Dec 3, 2017

View reviewed changes

bbatsov mentioned this pull request Jan 1, 2018

Make three font-lock faces customizable #466

Closed

bbatsov reviewed Jan 23, 2018

View reviewed changes

Bost added 2 commits March 12, 2018 21:49

Comment out failing tests

419c8a3

Numerous font-locking fixes and improvements

d6abf37

* s/clojure-interop-method-face/font-lock-type-face * keyword font-locking * visual examples w/ ert tests * fix failing ert tests

bbatsov merged commit d1e0a6e into clojure-emacs:master Mar 13, 2018

Bost deleted the plastic-surgery branch March 14, 2018 09:46

bbatsov mentioned this pull request Mar 14, 2018

Missing font-locking #474

Closed

vspinu added a commit to vspinu/clojure-mode that referenced this pull request Oct 24, 2018

Fix font-lock of type hints

f69ac8c

clojure-emacs#462

bbatsov pushed a commit that referenced this pull request Oct 24, 2018

Fix font-lock of type hints

dd8a193

#462

Uh oh!

No special font-locking for mixedCase, CamelCase #462

No special font-locking for mixedCase, CamelCase #462

Uh oh!

Conversation

Bost commented Nov 6, 2017

Uh oh!

bbatsov commented Nov 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bost commented Nov 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbatsov commented Dec 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbatsov commented Jan 1, 2018

Uh oh!

Bost commented Jan 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bost commented Jan 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbatsov commented Jan 23, 2018

Uh oh!

Bost commented Jan 27, 2018

Uh oh!

bbatsov commented Jan 28, 2018

Uh oh!

bbatsov commented Mar 12, 2018

Uh oh!

Bost commented Mar 13, 2018

Uh oh!

bbatsov commented Mar 13, 2018

Uh oh!

vspinu commented Oct 24, 2018

Uh oh!

vspinu commented Oct 24, 2018

Uh oh!

bbatsov commented Oct 24, 2018

Uh oh!

Bost commented Nov 6, 2018

Uh oh!

vspinu commented Nov 6, 2018

Uh oh!

Uh oh!

Bost commented Jan 1, 2018 •

edited

Loading